v3 quality pass: architectural refactor & bug fixes & lint cleanup#174
Open
ocean wants to merge 16 commits into
Open
v3 quality pass: architectural refactor & bug fixes & lint cleanup#174ocean wants to merge 16 commits into
ocean wants to merge 16 commits into
Conversation
An uncapped io.Copy allowed a malicious or slow server to write an arbitrarily large file when running `ahoy config init <url>`, exhausting disk space. Wrap the response body with io.LimitReader before copying.
The Makefile injected these three symbols via -X ldflags but they were never declared in Go source, so the linker silently dropped them. Every shipped binary was missing build provenance metadata.
The previous append(command.Environ(), cmdEnvVars...) placed user vars last. Linux uses last-match semantics so it worked there, but macOS getenv(3) returns the first match, silently ignoring the overrides. Replace with a deduplicating merge: cmdEnvVars go first, then inherited entries are added only when their key is not already present.
setupApp() was calling rootCmd.Execute() and os.Exit(0) internally when no .ahoy.yml was found, bypassing main()'s pipe-drain goroutine and making setupApp() impossible to unit-test as a pure builder. It now returns rootCmd early and main() handles execution uniformly.
…onfig When an imported file had the wrong ahoyapi version the error embedded the global `sourcefile` (root config path) instead of the `file` argument passed to getConfig(), sending the user to the wrong file.
…efixes The prefix rewriter converted -- to a single -, corrupting the standard end-of-options sentinel before it reached Cobra. Any arguments after -- that look like flags would be incorrectly parsed as ahoy flags.
The live code path uses PrintConfigReport() which writes to stdout. PrintValidationIssues() was never called and wrote to stderr, so any future caller would silently break scripts that pipe ahoy config validate.
…hically String comparison caused rc10 < rc9 because "1" < "9". Split pre-release labels on "." and compare numeric-looking segments as integers so that rc10 > rc9 and 10 > 9 compare correctly.
On Windows the built binary is ahoy.exe, so `cp ahoy` silently failed. Use $(BINARY_NAME) which is set to ahoy.exe on Windows_NT and ahoy elsewhere.
Lines without '=' were passed verbatim into exec.Cmd.Env, silently creating invalid entries. A common cause is shell 'export KEY=VALUE' syntax which is not supported. Now logs a warning and skips the line.
The hand-rolled flag parser only recognised the space-separated forms (--file <value>, -f <value>). The equals form would have been treated as a command argument, corrupting the call silently.
Remove all mutable package-level globals (sourcefile, verbose, ahoyExecutable, importVisited, AhoyConf, versionFlagSet, helpFlagSet, bashCompletionFlagSet, invalidFlagError) and replace with an appState struct whose methods carry state through the call chain. main() creates a fresh appState and calls setupApp; tests create isolated appState instances, eliminating the save/restore patterns that caused test-order sensitivity.
Replace the ambiguous ("", nil) return from getConfigPath when no
.ahoy.yml is found with ("", errNoConfig). setupApp now tests with
errors.Is(err, errNoConfig) so "no config" is clearly distinct from
genuine filesystem errors, and the srcFile empty-string check is
removed.
…x issues - Check all cmd.Help() and rootCmd.Help() return values - Consolidate four MarkHidden calls into a loop with _ = discard - Log error from stderr drain goroutine io.Copy - Replace if/else if entrypoint placeholder swap with tagged switch (QF1003) - Use http.NewRequestWithContext instead of client.Get to satisfy noctx - Remove unnecessary string() and []byte() conversions in tests - Handle os.Chdir and testFile.Write return values in tests - Discard cmd.Execute() return in appRun test helper
Signed-off-by: Drew Robinson <drew.robinson@gmail.com>
Contributor
WalkthroughReplaces all package-level mutable globals in the ahoy CLI with a per-invocation ChangesappState Refactor, Validation Reporting, and Download Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config_init_test.go (1)
48-66: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression test for bodies larger than the download cap.
The suite currently validates status/scheme errors but not the new max-size guard. Please add a test server that returns >5 MB and assert
downloadFilereturns an error and does not finalise the destination file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config_init_test.go` around lines 48 - 66, Add a new test function (similar to TestDownloadFile_200Response) that validates the max-size guard for downloadFile. Create a test server that responds with a body larger than 5 MB, then call downloadFile with the test server URL and a temporary file path. Assert that downloadFile returns an error (indicating the download was rejected due to size), and verify using fileExists that the destination file was not created, ensuring the function properly rejects and does not finalize files exceeding the size limit.ahoy_test.go (1)
316-326:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
cmd.Execute()errors inappRuninstead of discarding them.Line 316 drops the execution error, so this helper can report success even when the command failed and wrote nothing to stderr. That can create false-positive test outcomes.
Suggested fix
cmd.SetArgs(cmdArgs) - _ = cmd.Execute() + execErr := cmd.Execute() @@ out, _ := io.ReadAll(r) errOut, _ := io.ReadAll(rErr) - // If there was an error output, include it + // Prefer command execution errors; include stderr context when present. + if execErr != nil { + if len(errOut) > 0 { + return string(out), fmt.Errorf("%w: %s", execErr, strings.TrimSpace(string(errOut))) + } + return string(out), execErr + } + + // Preserve existing behaviour for stderr output on success paths. if len(errOut) > 0 { return string(out), fmt.Errorf("%s", errOut) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ahoy_test.go` around lines 316 - 326, The cmd.Execute() call is discarding its error return value instead of capturing it, which can mask command failures in test results. Modify the appRun function to capture the error returned by cmd.Execute() instead of using the blank identifier, and then update the error handling logic to return an error if either cmd.Execute() returned an error OR if there is stderr output, ensuring both failure paths are properly reported to the test.
🧹 Nitpick comments (1)
config_validation.go (1)
111-144: 💤 Low valuePre-release segment comparison has an edge case when one version has more segments.
When comparing pre-release versions where one has more segments (e.g.,
v1.0.0-rc.1vsv1.0.0-rc.1.0), the shorter one will have empty string segments (s1ors2will be""). An empty string converts to 0 viastrconv.Atoiwith an error, so it falls through to string comparison where""< any non-empty string. This matches SemVer 2.0 spec where the shorter identifier would be less, but empty string conversion to 0 with error could be confusing. The current behaviour is correct but consider adding a comment explaining the empty-segment handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config_validation.go` around lines 111 - 144, The pre-release segment comparison logic handles an edge case where one version has fewer segments than the other, resulting in empty strings for missing segments. Add a clarifying comment before or near the string comparison section (the else block that compares s1 < s2) to explain that when strconv.Atoi fails on empty strings, the string comparison naturally handles the edge case correctly by treating empty segments as sorting lower than non-empty ones, which aligns with SemVer 2.0 specification. This will help future readers understand why this behavior is intentional and correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli_parsing_test.go`:
- Around line 38-43: The test in TestInitFlags creates a fresh AppState which
already has srcDir as an empty string by default, so the assertion on line 41
does not meaningfully verify that the reset logic is working. Before calling
initFlags on the AppState s, set srcDir to a non-empty value (such as a test
path like "/some/path") so that the subsequent assertion actually validates that
initFlags properly resets the srcDir field instead of just confirming the
initial zero-value state.
In `@config_init.go`:
- Around line 60-61: The downloadFile function in config_init.go caps reads at
maxDownloadBytes using io.LimitReader but fails to detect when the actual
response exceeds this limit. If the server sends more than 5 MB, io.Copy will
silently truncate the content and return success, resulting in a partial YAML
file being written. After the io.Copy call with the limited reader on line 61,
add a check to detect oversized responses by attempting to read one more byte
from the original resp.Body. If this read succeeds (indicates more data exists),
return an error stating the response exceeded the maximum allowed size instead
of silently accepting the truncated content.
---
Outside diff comments:
In `@ahoy_test.go`:
- Around line 316-326: The cmd.Execute() call is discarding its error return
value instead of capturing it, which can mask command failures in test results.
Modify the appRun function to capture the error returned by cmd.Execute()
instead of using the blank identifier, and then update the error handling logic
to return an error if either cmd.Execute() returned an error OR if there is
stderr output, ensuring both failure paths are properly reported to the test.
In `@config_init_test.go`:
- Around line 48-66: Add a new test function (similar to
TestDownloadFile_200Response) that validates the max-size guard for
downloadFile. Create a test server that responds with a body larger than 5 MB,
then call downloadFile with the test server URL and a temporary file path.
Assert that downloadFile returns an error (indicating the download was rejected
due to size), and verify using fileExists that the destination file was not
created, ensuring the function properly rejects and does not finalize files
exceeding the size limit.
---
Nitpick comments:
In `@config_validation.go`:
- Around line 111-144: The pre-release segment comparison logic handles an edge
case where one version has fewer segments than the other, resulting in empty
strings for missing segments. Add a clarifying comment before or near the string
comparison section (the else block that compares s1 < s2) to explain that when
strconv.Atoi fails on empty strings, the string comparison naturally handles the
edge case correctly by treating empty segments as sorting lower than non-empty
ones, which aligns with SemVer 2.0 specification. This will help future readers
understand why this behavior is intentional and correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd22df98-7e54-494d-a390-6e4071171d41
📒 Files selected for processing (12)
Makefileahoy.goahoy_test.gocli_parsing_test.goconfig.goconfig_init.goconfig_init_test.goconfig_validation.godescription_test.goflag.gotests/missing-cmd.batswindows_test.go
Comment on lines
+38
to
43
| // Test with empty flags - srcDir should be reset to empty string. | ||
| s := newAppState() | ||
| s.initFlags([]string{}) | ||
| if s.srcDir != "" { | ||
| t.Error("Expected srcDir to be reset to empty string") | ||
| } |
Contributor
There was a problem hiding this comment.
Make the srcDir reset assertion meaningful in TestInitFlags.
Line 39 initialises a fresh zero-value state, so the check on Line 41 would still pass if reset logic stopped clearing srcDir.
Suggested fix
// Test with empty flags - srcDir should be reset to empty string.
s := newAppState()
+ s.srcDir = "stale-value"
s.initFlags([]string{})
if s.srcDir != "" {
t.Error("Expected srcDir to be reset to empty string")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Test with empty flags - srcDir should be reset to empty string. | |
| s := newAppState() | |
| s.initFlags([]string{}) | |
| if s.srcDir != "" { | |
| t.Error("Expected srcDir to be reset to empty string") | |
| } | |
| // Test with empty flags - srcDir should be reset to empty string. | |
| s := newAppState() | |
| s.srcDir = "stale-value" | |
| s.initFlags([]string{}) | |
| if s.srcDir != "" { | |
| t.Error("Expected srcDir to be reset to empty string") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_parsing_test.go` around lines 38 - 43, The test in TestInitFlags creates
a fresh AppState which already has srcDir as an empty string by default, so the
assertion on line 41 does not meaningfully verify that the reset logic is
working. Before calling initFlags on the AppState s, set srcDir to a non-empty
value (such as a test path like "/some/path") so that the subsequent assertion
actually validates that initFlags properly resets the srcDir field instead of
just confirming the initial zero-value state.
Comment on lines
+60
to
+61
| const maxDownloadBytes = 5 * 1024 * 1024 // 5 MB - generous for any YAML config file | ||
| if _, err = io.Copy(out, io.LimitReader(resp.Body, maxDownloadBytes)); err != nil { |
Contributor
There was a problem hiding this comment.
Detect oversized responses instead of silently truncating.
Line 60 currently caps reads, but Line 61 treats a clipped body as success. If the server sends more than 5 MB, downloadFile can write a partial YAML and still return nil, which breaks the “safe init” contract.
Proposed fix
- const maxDownloadBytes = 5 * 1024 * 1024 // 5 MB - generous for any YAML config file
- if _, err = io.Copy(out, io.LimitReader(resp.Body, maxDownloadBytes)); err != nil {
+ const maxDownloadBytes = 5 * 1024 * 1024 // 5 MB - generous for any YAML config file
+ written, err := io.Copy(out, io.LimitReader(resp.Body, maxDownloadBytes+1))
+ if err != nil {
out.Close()
return fmt.Errorf("failed to write file %s: %v", destPath, err)
}
+ if written > maxDownloadBytes {
+ out.Close()
+ return fmt.Errorf("failed to download file: response exceeds %d bytes", maxDownloadBytes)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config_init.go` around lines 60 - 61, The downloadFile function in
config_init.go caps reads at maxDownloadBytes using io.LimitReader but fails to
detect when the actual response exceeds this limit. If the server sends more
than 5 MB, io.Copy will silently truncate the content and return success,
resulting in a partial YAML file being written. After the io.Copy call with the
limited reader on line 61, add a check to detect oversized responses by
attempting to read one more byte from the original resp.Body. If this read
succeeds (indicates more data exists), return an error stating the response
exceeded the maximum allowed size instead of silently accepting the truncated
content.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch resolves 19 issues identified across several in-depth agent-assisted code reviews, covering correctness bugs, architectural problems, and test quality across the ahoy CLI codebase.
Architectural changes
appStatestruct - removes all seven mutable package-level globals (sourcefile,verbose,AhoyConf,importVisited, etc.) and converts functions to methods. Each invocation and test now gets an isolated instance; no more save/restore patterns in tests.setupApp()no longer owns execution, movedrootCmd.Execute()andos.Exit()calls out ofsetupApp()intomain(), making the function testable and fixing a pipe-drain bypass on the no-config path.getConfigPath()sentinel error, returns("", errNoConfig)instead of("", nil)when no.ahoy.ymlis found, giving callers an idiomatic error check rather than a string-empty check.Bug fixes
getenvsemantics)ahoy initto prevent unbounded response body readsGitCommit,GitBranch,BuildTimevars declared soldflagsinjection is no longer silently droppedgetConfig()error message names the failing import file, not the root confignormaliseLongFlagPrefixes()no longer corrupts the bare--end-of-options separatorrc10 > rc9)installtarget uses$(BINARY_NAME)instead of hardcodedahoygetEnvironmentVars()warns and skips malformed lines (noKEY=VALUEseparator)PrintValidationIssues()function removedTest & lint
appRun()test helper handles--file=value/-f=valueequals-form flagsgolangci-linterrcheck, unconvert, staticcheck (QF1003), and noctx findings resolvedNewRequestWithContextfor proper context propagationTest status
gofmt,staticcheck,go vet: cleanSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores